Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement OpenVox support #165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bastelfreak
Copy link
Member

No description provided.

@bastelfreak bastelfreak self-assigned this Jan 24, 2025
@bastelfreak
Copy link
Member Author

I know that this isn't ready yet. The focus is on creating beaker jobs for puppet and openvox packages. unit tests will continue with the puppet gem.

@bastelfreak bastelfreak force-pushed the openvox2 branch 2 times, most recently from f0bf384 to 151acff Compare January 24, 2025 21:00
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is what I'd do. In the the what I'd like is a single variable (both for unit and acceptance, so really 2 vars) that generates the entire matrix. That can be both puppet and openvox. We exposed 1 variable that gha uses now. Compatible solution is to just expand that result, even if the name includes puppet. Incompatible is to have genetic names like puppet_metadata_unit_matrix or similar. Prefix it with the gem name and a suffix for the purpose.

Looking further ahead we probably also want to do similar things with unit tests to test with agent gems

@bastelfreak
Copy link
Member Author

want to do similar things with unit tests to test with agent gems

Yes. But since we don't have the gem published at the moment, it didn't have a high priority for me.

Compatible solution is to just expand that result, even if the name includes puppet

That's my idea. I want to expand it, if openvox is listed in the requirements. In that way it isn't a breaking change. And if individual modules don't want to/cannot support Perforce packages anymore, we drop the puppet key from the requirements. And if there's a situation where we don't want to use the Perforce packages in any module, we could just reduce the result again.

@ekohl
Copy link
Member

ekohl commented Jan 24, 2025

Right, so let's start 2 variable that are generic: puppet_metadata_unix_matrix and puppet_metadata_acceptance_matrix. Those generate a GHA compatible matrix based on metadata. How we exactly define is is an implementation detail. Then in gha-puppet we start using the new matrices.

On the implementation details, for acceptance I'd say it osrequirementversions. So centos 9 * {puppet,openvox} * 8 for example. It may be more complex, but that's up to this gem to decide. Similar for unit tests

Comment on lines 156 to 158
# Current latest major is 7. It is highly recommended that modules
# actually specify exact bounds, but this prevents an infinite loop.
end_major = (requirement.end == SemanticPuppet::Version::MAX) ? 7 : requirement.end.major
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks out of date, which is a problem with hardcoding magic numbers ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not worth the dependency, but we could depend on rspec-puppet-facts, which ships https://github.com/voxpupuli/rspec-puppet-facts/blob/master/ext/puppet_agent_components.json

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really not mind to see hard failures when an upper bound is not set.

@bastelfreak bastelfreak force-pushed the openvox2 branch 10 times, most recently from 6d72ccd to 1f335e8 Compare February 21, 2025 22:32
@bastelfreak bastelfreak changed the title Add support for OpenVox beaker jobs Implement OpenVox support Feb 22, 2025
@bastelfreak bastelfreak force-pushed the openvox2 branch 2 times, most recently from 15aaaf2 to ae38ddf Compare February 22, 2025 17:11
@bastelfreak bastelfreak marked this pull request as ready for review February 22, 2025 17:13
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of these things are me thinking out loud. I'd welcome counter arguments.

Comment on lines +121 to +122
# when we want to make this dynamic, switch to:
# "BEAKER_#{requirement_name.upcase}_COLLECTION"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud: today we use beaker_puppet_helpers and this variable configures that. If we look at the values it supports then there are things like puppet7, puppet8 and with https://github.com/voxpupuli/beaker_puppet_helpers/pull/56 also openvox8. I think looking at multiple env vars is going to be a lot more messy so I'm not sure I'd like it.

Perhaps beaker_puppet_helpers deserves a new name and once that's there we can pick a new env var. Or there are other solutions, but iterating env vars for supported values is something I'd like to avoid. Since it's an environment we control I'd even prefer an env var to indicate which requirement to look at.

One thing where we may find an issue is the distro version, but I doubt distros are going to package both Puppet and OpenVox.

Comment on lines 148 to 149
# @return [Array[Integer]] Supported major Puppet versions
# @see #requirements
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked having methods documented with yardoc tags.

@@ -48,15 +60,19 @@ def puppet_unit_test_matrix
end.compact
end

def beaker_os_releases(at = nil)
majors = puppet_major_versions
def beaker_os_releases(at = nil, requirement_name = 'openvox')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is this called without a value? I'd make it mandatory. It is breaking the API, but it's a private method anyway. This comes back to my original idea that at is always the last parameter.

Suggested change
def beaker_os_releases(at = nil, requirement_name = 'openvox')
def beaker_os_releases(requirement_name, at = nil)

puppet_major_versions
when 'openvox'
openvox_major_versions
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
end
else
raise Exception, "Unknown requirement '#{requirement_name}'"
end

beaker_test_matrix(at, 'openvox') + beaker_test_matrix(at, 'puppet')
end

def beaker_test_matrix(at, requirement_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then also keep at as the last argument here

Suggested change
def beaker_test_matrix(at, requirement_name)
def beaker_test_matrix(requirement_name, at)

matrix_include = []

beaker_os_releases(at) do |os, release, puppet_version|
next if puppet_version_below_minimum?(puppet_version[:value])
beaker_os_releases(at, requirement_name) do |os, release, puppet_version|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses puppet variable naming quite often. What is a good way to describe either Puppet or OpenVox? implementation feels long and generic, but it's the best I can come up with.

Comment on lines +152 to +153
# don't raise an exception
# raise Exception, "No #{requirement_name} requirement found" unless requirement
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of commented code. I wonder if we should separate out the concerns so you can do the exception handling.

Thinking out loud about APIs:

def puppet_major_versions
  requirement = requirements['puppet']
  return [] unless requirement

  major_version_for_requirement(requirement)
end

def puppet_major_versions!
  requirement = requirements['puppet']
  raise Exception, 'No Puppet requirement found' unless requirement

  major_version_for_requirement(requirement)
end

private

def major_version_for_requirement(requirement)
end

Not that you need puppet_major_versions! in the current iteration, but it's for an example.

Then taking that a step further, you can generalize it:

def major_versions(requirement_name)
  requirement = requirements[requirement_name]
  return [] unless requirement

  major_version_for_requirement(requirement)
end

def major_versions!(requirement_name)
  requirement = requirements[requirement_name]
  raise Exception, "No '#{requirement_name}' requirement found' unless requirement

  major_version_for_requirement(requirement)
end

private

def major_version_for_requirement(requirement)
end

I like that a lot more because it keeps the PuppetMetadata::Metadata class very generic without naming individual requirements. We can accept that PuppetMetadata::GithubActions is a lot more specific while keeping this metadata abstraction as pure as possible.

The one thing that isn't pure is the missing upper bound handling. Technically that's already sort of broken, but in theory it could use some constant that's like this:

REQUIREMENT_UPPER_BOUND_DEFAULTS = {
  'openvox' => 8,
  'puppet' => 8,
}

But I don't think requirement has a name so I'd postpone looking at it and keep the current implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this file BUILDS refers to what Puppet has AIO builds for. Should that have an extra dimension of requirement_name?

Verified

This commit was signed with the committer’s verified signature. The key has expired.
bastelfreak Tim Meusel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants